-
Notifications
You must be signed in to change notification settings - Fork 301
Support HA & slow proliferation of cached value - Scenario 2 + 3 #15916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1b07a61 to
d42f064
Compare
| if ( markedColumn ) { | ||
| this._defaultSortBy = markedColumn.name; | ||
| descending = markedColumn.defaultSortDescending; | ||
| descending = markedColumn.defaultSortDescending || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fixes a console.warning related to initial sort order being undefined in sortable table
| if (!canPagination) { | ||
| // Reduce the impact of the initial load, but only if we're not making a request | ||
| if (!canPagination || !sideNavServiceInitialized) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix multiple request to fetch clusters when using side nav to move around
- When changing page the header + top level menu component is recreated. The impact of this is reduced a bit given the
TopLevelMenuHelperServicesingleton, but multiple requests are still made - This is due to the
immediateon a number of watches in TopLevelMenu triggering new requests - This is now fixed by making an initial request up front (only when needed) and not triggering the watches when we don't need to
| private backOffId: string; | ||
| private classify: boolean; | ||
| private reactive: boolean; | ||
| private cachedRevision?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache revision and result given there might be future calls that want an older result set... which we ignore and pass the current later result set
| // 2. current version is older than target revision - reset previous (drop older requests with older revision, use new revision) | ||
| // 3. current version is same as target revision - we're retrying | ||
|
|
||
| if (currentRevision.isNewerThan(targetRevision)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do both
activeRevisionSt.isNewerThan(targetRevision)
cachedRevisionSt.isNewerThan(targetRevision)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell/utils/pagination-wrapper.ts
- needs to split them in to, one is an abort, the other is returning cached value
shell/plugins/steve/subscribe.js - doesn't need to split them (does not need to return a result)
| return; | ||
| } | ||
|
|
||
| if (targetRevision.isNewerThan(activeRevision)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targetRevision.isNewerThan(currentRevision)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pagination.wrapper needed changing, not this one
| id: safeBackOffId, | ||
| metadata: { revision }, | ||
| description: `Fetching resources for ${ type }. Triggered by web socket`, | ||
| canFn: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canFn in wrapper world should check if the watch is running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipping this
- check 1 - canBackOff requires reference to
this.$socket - check 2 - watchStarted check requires original params
c7f6fa8 to
7c78a03
Compare
- Add tests for SteveRevision handling undefined - add comments linking two implementations - better match HA revision checks between implementations - better backoff logging of revision: undefined - reduce dupe filter given reuse of opt
7c78a03 to
ab025cb
Compare
Summary
Fixes #15414
Occurred changes and/or fixed issues
This PR revolves around ensuring
Requests are made in two places
Major Points
fetchPageResourcesrequestTechnical notes summary
Areas or cases that should be tested
There's no easy way to manufacture scenarios 1, 2 and 3. I've being doing this manually by code change. Recording here for posterity
shell/plugins/steve/subscribe.js:watch. before checking if revision is undefined and for a specific resource set the revision in the watch request to 'abc'. do this for the first 5 times it's hit.shell/plugins/dashboard-store/actions.js:findPage. beforedispatch('request'addthrow { status: 400, code: 'unknown revision' };`. only call it second to fifth times (the initial call should succeed)shell/plugins/steve/subscribe.js:ws.resource.changes. before calling fetchResources and for a specific resource set msg.revision to 0. do this the first 2 times it's hitpodmanagement.cattle.io.clusterpodmanagement.cattle.io.clusterpodmanagement.cattle.io.clusterAlong with scenarios 2 + 3, plus the original 1, the angles I've looked at
Areas which could experience regressions
Screenshot/Video
Checklist
Admin,Standard UserandUser Base